Skip to content

MDEV-40634 Const MEMORY table's BLOB outlives the lock protecting it - #5490

Open
arcivanov wants to merge 1 commit into
MariaDB:bb-blob-main-montyfrom
arcivanov:MDEV-40634
Open

MDEV-40634 Const MEMORY table's BLOB outlives the lock protecting it#5490
arcivanov wants to merge 1 commit into
MariaDB:bb-blob-main-montyfrom
arcivanov:MDEV-40634

Conversation

@arcivanov

Copy link
Copy Markdown
Contributor

https://jira.mariadb.org/browse/MDEV-40634

The defect

A single-row table is read once during optimization and its row kept in
record[0] for the rest of the statement. JOIN::optimize_stage2() then
releases the lock on every const table, on the premise stated in its own
comment: "It's safe to ignore result code as all tables where opened for read
only."

That premise assumes a read leaves a copy of the row behind. MEMORY with a
blob does not. hp_read_blobs() answers the read by pointing record[0] at the
blob data inside HP_SHARE rather than copying it, so from the moment the lock
is dropped another connection is free to overwrite, free or recycle those bytes
— and the statement goes on reading them.

Observed on 06cfb8d0c36 (this PR's base), with LENGTH() still reporting 4000
throughout because the length lives in record[0] and only the data moved:

writer does const row returns
UPDATE then INSERT (blocks recycled) zzzz… — another connection's data
DELETE FROM t1 then INSERT (blocks freed) poison bytes — freed memory

CHECK TABLE reports OK in both cases: the table is undamaged, the corruption
is entirely in the reader's row.

Pre-existing in the shipped HEAP-blob feature, not introduced by any of the
open fix branches.

The fix

GET_LOCK_SKIP_ZERO_COPY_ROWS drops such tables from the lock set
get_lock_data() builds, exactly as GET_LOCK_SKIP_SEQUENCES already does, and
the const-table unlock passes it.

The skip is opt-in rather than a rule because mysql_lock_remove() also reaches
mysql_unlock_some_tables(), and there the unlock is permanent and must not be
skipped.

Cost: a MEMORY blob const table now stays read-locked for the whole statement
and blocks writers, which is the price every non-const MEMORY table already
pays.

Only the early-unlock path is affected

The three functions that release a lock before the end of a statement were
audited; this is the only unsafe caller.

  • mysql_unlock_read_tables()JOIN::join_free, safe: cleanup(full) runs
    first and requires all cursors closed.
  • mysql_lock_remove() — every call site immediately follows with
    ha_close() / close_thread_table() / drop_temporary_table(), so no
    record[0] survives.

Non-transactional temporary tables need no handling: get_lock_data() already
drops them from the lock set, so they can neither be early-unlocked nor raced.

Testing

mysql-test/suite/heap/blob_const_unlock.test, deterministic, ~20 ms, covering
both ways the memory changes hands.

Unit-test-first verified: against the pre-fix binary the test reproduces both
failure modes, and the only difference between the pre-fix and fixed runs is the
two blob reads.

The reader is parked with GET_LOCK() rather than with a stored function. This
is load-bearing: a stored function puts the statement into prelocked mode
(DML_prelocking_strategy::handle_routine() sets need_prelocking for any
function, even one that touches no tables), and the const-table unlock is
guarded by !thd->locked_tables_mode, so a function-based test passes
vacuously against the broken build.

  • heap.blob_const_unlock — 20/20 repeats
  • full main,heap sweep — 1426/1426, zero retries
  • heap unit tests — 9/9

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Aug 5, 2026

@gkodinov gkodinov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. This is a preliminary review.

For the record: the test fails on embedded buildbot hosts. Please rectify. Otherwise, no further comments.

@gkodinov gkodinov self-assigned this Aug 5, 2026
A single-row table is read once during optimization and its row kept in
`record[0]` for the rest of the statement.  `JOIN::optimize_stage2()`
then releases the lock on every const table, on the premise stated in
its own comment: *"It's safe to ignore result code as all tables where
opened for read only."*

That premise assumes a read leaves a **copy** of the row behind.  MEMORY
with a blob does not.  `hp_read_blobs()` answers the read by pointing
`record[0]` at the blob data inside `HP_SHARE` rather than copying it,
so from the moment the lock is dropped another connection is free to
overwrite, free or recycle those bytes -- and the statement goes on
reading them.  The result is a const table whose value changes in the
middle of the statement using it, and a read of freed memory.

Let a caller that keeps reading a row after the unlock ask for such
tables to be left alone.  `GET_LOCK_SKIP_ZERO_COPY_ROWS` drops them
from the lock set `get_lock_data()` builds, exactly as
`GET_LOCK_SKIP_SEQUENCES` already does, and the const-table unlock in
`JOIN::optimize_stage2()` passes it.

The skip has to be opt-in rather than a rule.  `mysql_lock_remove()`
also reaches `mysql_unlock_some_tables()`, and there the unlock is
permanent and must not be skipped.

A MEMORY blob const table now stays read-locked for the whole statement
and blocks writers, which is the price every non-const MEMORY table
already pays.

The regression test parks the reader with `GET_LOCK()` rather than with
a stored function.  A stored function puts the statement into prelocked
mode, and the const-table unlock is skipped entirely in that mode, so
the code path under test would never run.

The gate that parks it is taken with `--disable_ps2_protocol` in force.
`--ps-protocol` executes every complete `SELECT` twice and compares the
two result sets, and `GET_LOCK()` is recursive, so a doubled
acquisition would outlive the single `RELEASE_LOCK()` that opens the
gate again.

@gkodinov gkodinov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks. Please stand by for the final review.

@gkodinov
gkodinov requested a review from montywi August 5, 2026 11:39
@gkodinov gkodinov assigned montywi and unassigned gkodinov Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

3 participants